-
Notifications
You must be signed in to change notification settings - Fork 27.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Scroll-y menus #62819
Scroll-y menus #62819
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually seems to work quite well.
@sbatten please bring up to date with master |
Yeah possibly, but I prefer to apply the changes on top of master. And besides, there is a merge conflict too. |
@bpasero updated and conflicts resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbatten yeah works nicely, some feedback:
- looks like the onscroll emitter is never disposed
- the scrollbar is a bit hard to see in high contrast themes (see below)
- the scrollbar should probably always be visible even when the mouse is not over the menu for accessibility reasons (see how quick open always shows a scrollbar)
- the scrollbar does not update its position when I navigate using the keyboard
When a menu is created, set a max height and use scrollable element. There is room for improvement but this only takes a visible effect when needed and in that case, it is certainly better than the current situation.
Not having used the scrollableelement class before and due the strange behavior of overflow with regards to submenus, please review carefully.
fixes #52533